rustdoc: clean up generic impls #52827

GuillaumeGomez · 2018-07-29T15:02:47Z

rust-highfive · 2018-07-29T15:37:45Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:32:11]    Compiling aho-corasick v0.6.6
[00:32:15]    Compiling tempfile v3.0.2
[00:32:33]    Compiling minifier v0.0.14
[00:32:35]    Compiling rustdoc v0.0.0 (file:///checkout/src/librustdoc)
[00:32:36] error: trait objects without an explicit `dyn` are deprecated
[00:32:36]   --> librustdoc/clean/def_ctor.rs:19:14
[00:32:36]    |
[00:32:36] 19 | where F: Fn(&Fn(DefId) -> Def) -> Vec<Item> {
[00:32:36]    |              ^^^^^^^^^^^^^^^^ help: use `dyn`: `dyn Fn(DefId) -> Def`
[00:32:36]    |
[00:32:36]    = note: requested on the command line with `-D bare-trait-objects`
[00:32:36] 
[00:32:36] error: trait objects without an explicit `dyn` are deprecated
[00:32:36]   --> librustdoc/clean/def_ctor.rs:56:14
[00:32:36]    |
[00:32:36] 56 | where F: Fn(&Fn(DefId) -> Def, String) -> Vec<Item> {
[00:32:36]    |              ^^^^^^^^^^^^^^^^ help: use `dyn`: `dyn Fn(DefId) -> Def`
travis_time:end:19d124f0:start=1532876703082642932,finish=1532878662741449220,duration=1959658806288

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0acacda8

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

GuillaumeGomez · 2018-07-31T08:33:43Z

cc @eddyb

eddyb · 2018-07-31T09:20:08Z

src/librustdoc/clean/blanket_impl.rs

+use self::finder_trait::Finder;
+
+pub struct BlanketImplFinder<'a, 'tcx: 'a, 'rcx: 'a> {
+    pub cx: &'a core::DocContext<'a, 'tcx, 'rcx>,


Why not just add methods to DocContext? Also, I'd rename the methods so they all start with get_blanket_impls_for_... instead.

Good point. Moving into DocContext.

Actually it's weird to move it directly into DocContext... Why would the DocContext provide for blanket impls?

Because it's the global object of rustdoc and this is part of what rustdoc needs to do.

Whenever this PR is merged, i'd like to move the auto/blanket impl collection (as well as all the other explicit impl collection) into an early pass, so the code can be isolated over there instead.

eddyb · 2018-07-31T09:22:24Z

src/librustdoc/clean/def_ctor.rs

+                                   def_id: DefId,
+                                   callback: &F,
+) -> Vec<Item>
+where F: Fn(& dyn Fn(DefId) -> Def) -> Vec<Item> {


You can do this generally by replacing Vec<Item> with R. Also you can get the DefId from TyAdt , so why not just return an Option<Def>, with no callback?

The filtering is done before.

Also, why would it be better to replace Vec<Item> with R?

Because Vec<Item> is very arbitrary and the function doesn't have anything to do with Vec<Item>, that's just what the closures it's used with deal with.

Returning Option<Def> is probably a better idea.

This comment is still outstanding, but has been hidden by future commits editing the area around it.

I would prefer to see a generic return value - that's how these callback methods tend to work. However, since there is that arm returning Vec::new(), we should either depend on the return value implementing Default, or restructure it so that a callback is not necessary.

eddyb · 2018-07-31T09:23:04Z

src/librustdoc/clean/def_ctor.rs

+
+use super::*;
+
+pub fn get_def_ctor_from_def_id<F>(cx: &DocContext,


This should have a name like get_def_from_ty. I'm guessing this is mostly for aliases?

Mostly yes.

eddyb · 2018-07-31T11:43:35Z

src/librustdoc/clean/finder_trait.rs

+    // Instead, we construct 'fake' def ids, which start immediately after the last DefId in
+    // DefIndexAddressSpace::Low. In the Debug impl for clean::Item, we explicitly check for fake
+    // def ids, as we'll end up with a panic if we use the DefId Debug impl for fake DefIds
+    fn next_def_id(&self, crate_num: CrateNum) -> DefId {


Why would you need new DefIds?!

To avoid them getting filtered out because of duplicates.

Ah hold on, it's required for the auto traits (sync and send) but not be for blanket impls.

Still, why?

No it's required for blanket impls as well otherwise they're counted as duplicates.

You should change the deduplication logic instead, then!

Yep but nothing to do with this PR. :)

Every clean::Item needs a DefId. As the synthetic impls for auto traits (and hopefully eventually, blanket traits too) invent an impl out of thin air, one needs to be created so that the Item can hold it.

The real question is whether we can get away with only creating clean::Impl structs, and ignoring/inferring the fields on Item, like its attributes, visibility, stability, or deprecation. These can probably be inferred from the trait itself (or the blanket impl being applied), so that would help to prevent us from requiring new DefIds.

QuietMisdreavus

Thanks so much for refactoring this! I have a couple comments.

QuietMisdreavus · 2018-08-02T22:14:31Z

src/librustdoc/clean/finder_trait.rs

+use super::*;
+
+pub trait Finder<'a, 'tcx: 'a, 'rcx: 'a> {
+    fn get_cx(&self) -> &DocContext<'a, 'tcx, 'rcx>;


I'm wondering why these methods need to exist on a trait like this, if all the implementor needs to do is supply a DocContext. Could we instead add them to DocContext?

In this case, it's just to be able to implement some methods directly inside the trait (and therefore avoid code duplication).

I like to keep things grouped by functionalities. Also, AutoTraitFinder requires an additional field to work so I think merging it inside DocContext would be a mistake. Therefore, I think keeping the BlanketImplFinder struct on its own would be better. However, if you prefer me to merge it inside DocContext, I'll.

(To pull the conversation out of discord...)

I'm not talking about AutoTraitFinder or BlanketImplFinder. Those are distinct enough to be on their own. I'm thinking more about just the provided methods on the Finder trait itself.

QuietMisdreavus · 2018-08-03T18:10:58Z

src/test/rustdoc/primitive-generic-impl.rs

+// we need to reexport something from libstd so that `all_trait_implementations` is called.
+pub use std::string::String;
+
+include!("primitive/primitive-generic-impl.rs");


Why did you use include!() here? Did it affect the result of this test?

I replicated how it was performed in libstd. Since it's the main target of this fix, I thought it was the best way.

QuietMisdreavus · 2018-08-03T18:17:07Z

src/librustdoc/clean/finder_trait.rs

+    // Instead, we construct 'fake' def ids, which start immediately after the last DefId in
+    // DefIndexAddressSpace::Low. In the Debug impl for clean::Item, we explicitly check for fake
+    // def ids, as we'll end up with a panic if we use the DefId Debug impl for fake DefIds
+    fn next_def_id(&self, crate_num: CrateNum) -> DefId {


Every clean::Item needs a DefId. As the synthetic impls for auto traits (and hopefully eventually, blanket traits too) invent an impl out of thin air, one needs to be created so that the Item can hold it.

The real question is whether we can get away with only creating clean::Impl structs, and ignoring/inferring the fields on Item, like its attributes, visibility, stability, or deprecation. These can probably be inferred from the trait itself (or the blanket impl being applied), so that would help to prevent us from requiring new DefIds.

QuietMisdreavus · 2018-08-04T20:58:48Z

Outstanding comments:

The callback in get_def_from_(def|node)_id can be expressed generically, with the closure and the function returning some R: Default, or otherwise refactoring the function so that the closure is not necessary. I'm okay leaving this as-is, since it can be modified without moving as much code as the rest of this PR already does.
We should investigate whether we need to store full clean::Items for synthetic impls. If we can get away without storing a full Item, we don't have to generate fake DefIds either, which cleans up a major point of contention from @eddyb. While this would be a great thing to do, this PR is already big enough (and apparently has already had to undergo a rebase for a merge conflict). It can be handled separately.

Otherwise, this looks ready to go! If we can get this landed, we can build farther refactors on top of it.

@bors r+

bors · 2018-08-04T20:58:49Z

📌 Commit d57f3a6 has been approved by QuietMisdreavus

bors · 2018-08-04T21:20:33Z

⌛ Testing commit d57f3a6 with merge 667fdc1...

@QuietMisdreavus

rustdoc: clean up generic impls r? @QuietMisdreavus

bors · 2018-08-04T23:20:57Z

☀️ Test successful - status-appveyor, status-travis
Approved by: QuietMisdreavus
Pushing 667fdc1 to master...

rust-highfive assigned QuietMisdreavus Jul 29, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 29, 2018

QuietMisdreavus changed the title ~~Generic impls~~ rustdoc: clean up generic impls Jul 29, 2018

eddyb reviewed Jul 31, 2018

View reviewed changes

GuillaumeGomez force-pushed the generic-impls branch from 0486913 to 26b387a Compare July 31, 2018 20:41

QuietMisdreavus reviewed Aug 3, 2018

View reviewed changes

GuillaumeGomez added 7 commits August 4, 2018 11:02

Clean generic impls code

4aba7de

Fix primitive blanket impls not showing up

8301081

Strengthen tests

e0f8f87

Fix dyn objects

2758de4

Improve functions naming

fc81adb

Remove FinderTrait and move its functions into DocContext

0bff861

Update to master

d57f3a6

GuillaumeGomez force-pushed the generic-impls branch from b006fc2 to d57f3a6 Compare August 4, 2018 09:41

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 4, 2018

bors added a commit that referenced this pull request Aug 4, 2018

Auto merge of #52827 - GuillaumeGomez:generic-impls, r=QuietMisdreavus

667fdc1

rustdoc: clean up generic impls r? @QuietMisdreavus

bors merged commit d57f3a6 into rust-lang:master Aug 4, 2018

GuillaumeGomez deleted the generic-impls branch August 5, 2018 12:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: clean up generic impls #52827

rustdoc: clean up generic impls #52827

GuillaumeGomez commented Jul 29, 2018

rust-highfive commented Jul 29, 2018

GuillaumeGomez commented Jul 31, 2018

eddyb Jul 31, 2018

GuillaumeGomez Jul 31, 2018

GuillaumeGomez Jul 31, 2018

eddyb Jul 31, 2018

QuietMisdreavus Aug 4, 2018

eddyb Jul 31, 2018

GuillaumeGomez Jul 31, 2018

GuillaumeGomez Jul 31, 2018

eddyb Jul 31, 2018

QuietMisdreavus Aug 3, 2018

eddyb Jul 31, 2018

GuillaumeGomez Jul 31, 2018

eddyb Jul 31, 2018

GuillaumeGomez Jul 31, 2018

GuillaumeGomez Jul 31, 2018

eddyb Jul 31, 2018

GuillaumeGomez Jul 31, 2018

eddyb Aug 1, 2018

GuillaumeGomez Aug 1, 2018

QuietMisdreavus Aug 3, 2018

QuietMisdreavus left a comment

QuietMisdreavus Aug 2, 2018

GuillaumeGomez Aug 3, 2018

QuietMisdreavus Aug 3, 2018

QuietMisdreavus Aug 3, 2018

GuillaumeGomez Aug 3, 2018

QuietMisdreavus Aug 3, 2018

QuietMisdreavus commented Aug 4, 2018

bors commented Aug 4, 2018

bors commented Aug 4, 2018

bors commented Aug 4, 2018


		use super::*;

		pub fn get_def_ctor_from_def_id<F>(cx: &DocContext,

rustdoc: clean up generic impls #52827

rustdoc: clean up generic impls #52827

Conversation

GuillaumeGomez commented Jul 29, 2018

rust-highfive commented Jul 29, 2018

GuillaumeGomez commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuietMisdreavus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuietMisdreavus commented Aug 4, 2018

bors commented Aug 4, 2018

bors commented Aug 4, 2018

bors commented Aug 4, 2018